-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
node-report: meld into core #22712
node-report: meld into core #22712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start. Needs a bit of work. Left some initial comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gireeshpunathil! Did a cursory glance over and left some comments. Will follow up with a proper technical review.
Docs should note that this feature is experimental (at least initially). I also have a preference for calling the report diagnostics report
(or something similar) instead of node-report
in core as I find the node-
prefix redundant.
What part of the tier1 status requires something to be part of Node.js' core? |
For releasers: This was landed in 01cd219...55e0ad9 We also may want to add a note to the entry for the first commit in the changelog, because the commit title tells very little about the change itself. |
@gireeshpunathil I don’t know where – I think here is fine? And basically I’m just looking for a very short description of the commit that would fit into our one-line changelog, to explain the commit message title, e.g. |
added the one liner to the head or the PR description, as well as to the bottom of the main commit. |
Make node-report part of core runtime because: 1. When enabled, node-report significantly helps root cause various types of problems, including support issues sent to the various repos of the Node.js organization. 2. The requirement of explicitly adding the dependency to node-report in user applications often represents a blocker to adoption. Major deviation from the module version of the node-report is that the report is generated in JSON format, as opposed to human readable text. No new functionalities have been added, changes that are required for melding it as a built-in capability has been affected on the module version of node-report (https://github.com/nodejs/node-report) Co-authored-by: Bidisha Pyne <[email protected]> Co-authored-by: Howard Hellyer <[email protected]> Co-authored-by: Jeremiah Senkpiel <[email protected]> Co-authored-by: Julian Alimin <[email protected]> Co-authored-by: Lakshmi Swetha Gopireddy <[email protected]> Co-authored-by: Manusaporn Treerungroj <[email protected]> Co-authored-by: Michael Dawson <[email protected]> Co-authored-by: Richard Chamberlain <[email protected]> Co-authored-by: Richard Lau <[email protected]> Co-authored-by: Sam Roberts <[email protected]> Co-authored-by: Vipin Menon <[email protected]> PR-URL: #22712 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
a separate section added for node-report at top level main documentation added to doc/api/report.md API documentation added to doc/api/process.md PR-URL: #22712 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
One test per each API, so that additional tests in future are modular. test/common/report.js contain common functions that tests leverage. PR-URL: #22712 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
Notable Changes: * events: * For unhandled `error` events with an argument that is not an `Error` object, the resulting exeption will have more information about the argument. #25621 * child_process: * When the `maxBuffer` option is passed, `stdout` and `stderr` will be truncated rather than unavailable in case of an error. #24951 * policy: * Experimental support for module integrity checks through a manifest file is implemented now. #23834 * n-api: * The `napi_threadsafe_function` feature is now stable. #25556 * report: * An experimental diagnostic API for capturing process state is available as `process.report` and through command line flags. #22712 * tls: * `tls.connect()` takes a `timeout` option analogous to the `net.connect()` one. #25517 * worker: * `process.umask()` is available as a read-only function inside Worker threads now. #25526 * An `execArgv` option that supports a subset of Node.js command line options is supported now. #25467 PR-URL: #25687
Notable Changes: * events: * For unhandled `error` events with an argument that is not an `Error` object, the resulting exeption will have more information about the argument. #25621 * child_process: * When the `maxBuffer` option is passed, `stdout` and `stderr` will be truncated rather than unavailable in case of an error. #24951 * policy: * Experimental support for module integrity checks through a manifest file is implemented now. #23834 * n-api: * The `napi_threadsafe_function` feature is now stable. #25556 * report: * An experimental diagnostic API for capturing process state is available as `process.report` and through command line flags. #22712 * tls: * `tls.connect()` takes a `timeout` option analogous to the `net.connect()` one. #25517 * worker: * `process.umask()` is available as a read-only function inside Worker threads now. #25526 * An `execArgv` option that supports a subset of Node.js command line options is supported now. #25467 PR-URL: #25687
process.on('somesignal', ...) semantics expect the process to catch the signal and invoke the associated handler. `setupSignalHandlers` perform the additional task of preparing the libuv signal handler and associate it with the event handler. It is possible that by the time this is setup there could be pre-existing registrations that pre-date this setup in the boot sequence. So rearm pre-existing signal event registrations to get upto speed. Ref: #22712 (comment) PR-URL: #24651 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
process.on('somesignal', ...) semantics expect the process to catch the signal and invoke the associated handler. `setupSignalHandlers` perform the additional task of preparing the libuv signal handler and associate it with the event handler. It is possible that by the time this is setup there could be pre-existing registrations that pre-date this setup in the boot sequence. So rearm pre-existing signal event registrations to get upto speed. Ref: #22712 (comment) PR-URL: #24651 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Remove `Node report in core` from Current list of initiatives, as this is achieved. Report has landed. Refs: nodejs/node#22712
Remove `Node report in core` from Current list of initiatives, as this is achieved. Report has landed. Refs: nodejs/node#22712 PR-URL: nodejs#286 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesThis PR adds the the
process.report
tool.When enabled, node-report significantly helps root cause various types of problems, including support issues sent to the various repos of the Node.js organization
The requirement of explicitly adding the dependency to node-report in users' applications often represents a blocker to adoption and thus cancels 1) benefits
No new functionalities have been added, changes that are required for melding it as a built-in capability has been affected on the module version of node-report (https://github.com/nodejs/node-report)
Changes at a glance:
util
module. APIs are enabled by default.--without-node-report
Refs: #19661
Refs: #18760
Refs: nodejs/node-report#103
additional /cc
@rnchamberlain : original author of this component
@richardlau : previous work on merging this into core
@mhdawson : orignal requirement raiser through nodejs/node-report#110
@nodejs/diagnostics : in relation to nodejs/TSC#586
@jasnell @rvagg @joyeecheung : in relation to their specific requirements on interfaces /defaults (#19661)